Skip to content

Fix sfp test's starting index#4099

Merged
prgeor merged 4 commits intosonic-net:masterfrom
prgeor:sfp_index_fix
Aug 26, 2021
Merged

Fix sfp test's starting index#4099
prgeor merged 4 commits intosonic-net:masterfrom
prgeor:sfp_index_fix

Conversation

@prgeor
Copy link
Copy Markdown
Contributor

@prgeor prgeor commented Aug 20, 2021 •
edited
Loading

Description of PR

The current platform API tests for SFP uses indices starting from 0 which fails to return the correct SFP object for platforms having physical port number starting from 1. This is because get_port_map() -> config_facts.create_map() always creates the map with starting index 0.

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 202012

Approach

What is the motivation for this PR?

Fix issues #8435, #8436 buildimage repo

How did you do it?

To fix, this we now first find all Ethernet ports in the DUT and use redis-cli to get the corresponding 'index' (physical port number). from config DB. This should work for breakout ports and multi-asic as well.

How did you verify/test it?

Ran all tests under test_sfp.py on Arista 7050CX3

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@prgeor prgeor requested a review from sujinmkang as a code owner August 20, 2021 14:40
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Aug 20, 2021

This pull request introduces 1 alert when merging 1c9260d into fe644b8 - view on LGTM.com

new alerts:

  • 1 for Unused import

@prgeor
Copy link
Copy Markdown
Contributor Author

prgeor commented Aug 21, 2021

@rawal01 can you review?

@rawal01
Copy link
Copy Markdown
Contributor

rawal01 commented Aug 21, 2021

@prgeor
The change works in almost all cases but I see a case where if you do not have all interfaces used on your DUT configured, in that case compare_value_with_platform_facts will not return right values and test using this will fail (right now only test_get_name).

          def compare_value_with_platform_facts(self, key, value, sfp_idx, duthost):
               expected_value = None
               sfp_id = self.list_sfps.index(sfp_idx)
               if duthost.facts.get("chassis"):
                   expected_sfps = duthost.facts.get("chassis").get("sfps")
               if expected_sfps:
                   expected_value = expected_sfps[sfp_id].get(key)

this because your list_sfps will be just a subset of all interfaces while platform.json from which it is comparing always has all interface sfp names.
One more suggestion `physical_port_indices' can be moved to platform utils and can be used on other tests like test_sfps in platform_tests/api/test_chassis.py

@prgeor
Addressed review comments
5b12f54 
Signed-off-by: Prince George <prgeor@microsoft.com>
@prgeor prgeor requested a review from a team as a code owner August 23, 2021 17:02
@prgeor
Copy link
Copy Markdown
Contributor Author

prgeor commented Aug 23, 2021

@prgeor
The change works in almost all cases but I see a case where if you do not have all interfaces used on your DUT configured, in that case compare_value_with_platform_facts will not return right values and test using this will fail (right now only test_get_name).

          def compare_value_with_platform_facts(self, key, value, sfp_idx, duthost):
               expected_value = None
               sfp_id = self.list_sfps.index(sfp_idx)
               if duthost.facts.get("chassis"):
                   expected_sfps = duthost.facts.get("chassis").get("sfps")
               if expected_sfps:
                   expected_value = expected_sfps[sfp_id].get(key)

this because your list_sfps will be just a subset of all interfaces while platform.json from which it is comparing always has all interface sfp names.
One more suggestion `physical_port_indices' can be moved to platform utils and can be used on other tests like test_sfps in platform_tests/api/test_chassis.py

@prgeor
The change works in almost all cases but I see a case where if you do not have all interfaces used on your DUT configured, in that case compare_value_with_platform_facts will not return right values and test using this will fail (right now only test_get_name).

          def compare_value_with_platform_facts(self, key, value, sfp_idx, duthost):
               expected_value = None
               sfp_id = self.list_sfps.index(sfp_idx)
               if duthost.facts.get("chassis"):
                   expected_sfps = duthost.facts.get("chassis").get("sfps")
               if expected_sfps:
                   expected_value = expected_sfps[sfp_id].get(key)

this because your list_sfps will be just a subset of all interfaces while platform.json from which it is comparing always has all interface sfp names.
One more suggestion `physical_port_indices' can be moved to platform utils and can be used on other tests like test_sfps in platform_tests/api/test_chassis.py

@rawal01 I have addressed your comment. can you check and approve?

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Aug 23, 2021

This pull request introduces 4 alerts when merging 5b12f54 into 8ecfc14 - view on LGTM.com

new alerts:

  • 3 for Unused import
  • 1 for Unused local variable

@sujinmkang
Copy link
Copy Markdown
Contributor

sujinmkang commented Aug 23, 2021

@prgeor can you fix the lgtm issue while you're waiting for @rawal01 's review?

@rawal01
Copy link
Copy Markdown
Contributor

rawal01 commented Aug 23, 2021

@prgeor Thank you for changes it resolves the one case I mentioned and also taking care of test_chassis.

I would like suggest some changes def get_physical_port_indices(duthosts, enum_rand_one_per_hwsku_hostname, conn_graph_facts): be changed to `def get_physical_port_indices(duthost)' as duthost is already defined in from where we make a call to this fixture. Also conn_graph_facts is not longer used.

also in both test_chassis.py and test_sfp.py we do not to define physical_pot_indices any more seems uneccessary
@pytest.fixture(scope="module")
def physical_port_indices(duthosts, enum_rand_one_per_hwsku_hostname, conn_graph_facts):
return get_physical_port_indices(duthosts, enum_rand_one_per_hwsku_hostname, conn_graph_facts)

I think it is simpler to assign lit directly list_sfps = get_physical_port_indices(duthost) as fixture is returning a list we need.

@prgeor
Fix LGTM warnings
402ee50 
Signed-off-by: Prince George <prgeor@microsoft.com>
@prgeor
Copy link
Copy Markdown
Contributor Author

prgeor commented Aug 24, 2021

@prgeor Thank you for changes it resolves the one case I mentioned and also taking care of test_chassis.

I would like suggest some changes def get_physical_port_indices(duthosts, enum_rand_one_per_hwsku_hostname, conn_graph_facts): be changed to `def get_physical_port_indices(duthost)' as duthost is already defined in from where we make a call to this fixture. Also conn_graph_facts is not longer used.

also in both test_chassis.py and test_sfp.py we do not to define physical_pot_indices any more seems uneccessary
@pytest.fixture(scope="module")
def physical_port_indices(duthosts, enum_rand_one_per_hwsku_hostname, conn_graph_facts):
return get_physical_port_indices(duthosts, enum_rand_one_per_hwsku_hostname, conn_graph_facts)

I think it is simpler to assign lit directly list_sfps = get_physical_port_indices(duthost) as fixture is returning a list we need.

@rawal01 It will be easier if you can comment against the file/line number. I have made the changes and fixed LGTM warnings. get_physical_port_indices() is a time consuming function and hence i have wrapped around it in a pytest fixture with module scope.

@prgeor
Copy link
Copy Markdown
Contributor Author

prgeor commented Aug 24, 2021

@yxieca could you review

@rawal01
Copy link
Copy Markdown
Contributor

rawal01 commented Aug 24, 2021

@prgeor Yes makes sense the changes look good to me

@sujinmkang sujinmkang requested a review from yxieca August 24, 2021 14:35
yxieca
yxieca reviewed Aug 24, 2021
View reviewed changes
@prgeor
Use sonic-db-cli
3a7bd2b 
Signed-off-by: Prince George <prgeor@microsoft.com>
@prgeor prgeor merged commit cf7fddf into sonic-net:master Aug 26, 2021
@prgeor prgeor mentioned this pull request Aug 27, 2021
Closed
vmittal-msft pushed a commit to vmittal-msft/sonic-mgmt that referenced this pull request Sep 28, 2021
@prgeor @vmittal-msft
Fix sfp test's starting index (sonic-net#4099)
880d5c9 
* Fix sfp test's starting index

* Addressed review comments

Signed-off-by: Prince George <prgeor@microsoft.com>

* Fix LGTM warnings

Signed-off-by: Prince George <prgeor@microsoft.com>

* Use sonic-db-cli

Signed-off-by: Prince George <prgeor@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yxieca yxieca yxieca approved these changes

+1 more reviewer

@sujinmkang sujinmkang sujinmkang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rawal01 rawal01

Labels

Bug 🐛

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@prgeor @rawal01 @sujinmkang @yxieca